Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix ground primitives crossing IDL in 2D #6984

Merged
merged 1 commit into from
Aug 31, 2018
Merged

fix ground primitives crossing IDL in 2D #6984

merged 1 commit into from
Aug 31, 2018

Conversation

likangning93
Copy link
Contributor

@likangning93 likangning93 commented Aug 31, 2018 •
edited
Loading

Fixes #6951

In Sandcastle

@cesium-concierge
Copy link

cesium-concierge commented Aug 31, 2018 •
edited by likangning93
Loading

Thanks for the pull request @likangning93!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

likangning93
likangning93 commented Aug 31, 2018
View reviewed changes
Source/Shaders/ShadowVolumeAppearanceVS.glsl
idlSplitNewPlaneHiLow = vec2(WEST_MOST_X_HIGH - (EAST_MOST_X_HIGH - planes2D_high.x), WEST_MOST_X_LOW - (EAST_MOST_X_LOW - planes2D_low.x));
planes2D_high.x = czm_branchFreeTernary(idlSplit, idlSplitNewPlaneHiLow.x, planes2D_high.x);
planes2D_low.x = czm_branchFreeTernary(idlSplit, idlSplitNewPlaneHiLow.y, planes2D_low.x);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a branchless equivalent to:

if (planes2D_high.x > planes2D_high.w) {
    // If the primitive is split across the IDL
    if (position3DLow.y > 0.0) { // Comparison with position3DHigh may produce artifacts
        // If this vertex is on the east side of the IDL:
        // - existing "east" is on the wrong side of the world, far away
        // - set "east" as beyond the eastmost extent of the projection
        planes2D_high.w = EAST_MOST_X_HIGH - (WEST_MOST_X_HIGH - planes2D_high.w);
        planes2D_low.w = EAST_MOST_X_LOW - (WEST_MOST_X_LOW - planes2D_low.w);
    } else {
        // If this vertex is on the west side of the IDL:
        // - existing "west" is on the wrong side of the world, far away
        // - set "west" as beyond the westmost extent of the projection
        planes2D_high.x = WEST_MOST_X_HIGH - (EAST_MOST_X_HIGH - planes2D_high.x);
        planes2D_low.x = WEST_MOST_X_LOW - (EAST_MOST_X_LOW - planes2D_low.x);
    }
}

@likangning93
Copy link
Contributor Author

Gist of what the problem was:

  • when primitives are split over the IDL, the positions for "east" and "west" that get added to the batch table are actually flipped
  • so the "planes" used to compute texture coordinates/do culling are really silly

Gist of the fix:

  • we pass the furthest "west" and furthest "east" positions into the vertex shader as constants
  • In the vertex shader, detect IDL-split based on the batch table planes
  • In the vertex shader, determine which side of the IDL the vertex is on. It's guaranteed that all connected geometry will be on one side or the other because of the IDL offset in GeometryPipeline
  • In the vertex shader, move the plane that's "wrong" for this side of the primitive using the constants passed in.

@likangning93 likangning93 requested a review from bagnell August 31, 2018 14:53
@ggetz
Copy link
Contributor

ggetz commented Aug 31, 2018

Just a reminder that if we want this in for the next release, it should be merged today.

@bagnell
Copy link
Contributor

bagnell commented Aug 31, 2018

👍

Don't forget about the issue where the volumes need to be closed 😃

@bagnell bagnell merged commit 685e3f3 into master Aug 31, 2018
@bagnell bagnell deleted the groundRectangleIdl2D branch August 31, 2018 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bagnell bagnell Awaiting requested review from bagnell

Assignees
No one assigned
Labels
priority - next release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@likangning93 @cesium-concierge @ggetz @bagnell